Summary of Changes

Hello @julienrbrt, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the robustness of the SyncService's P2P initialization process. It introduces a mechanism to perform background retries for P2P synchronization if the initial attempt times out, preventing the service from getting stuck and ensuring eventual connectivity. This change improves the overall resilience and startup reliability of the synchronization component.

Highlights

• Background Context for SyncService: Introduced bgCtx and bgCancel fields to the SyncService struct to manage background operations, ensuring proper cancellation when the service stops.
• Refactored P2P Initialization Logic: Extracted the core logic for a single P2P initialization attempt into a new tryInit method, improving code modularity and reusability.
• Background P2P Initialization Retries: Implemented a new retryInitInBackground goroutine that is launched if the initial P2P synchronization times out. This goroutine continuously attempts to initialize the syncer in the background with exponential backoff, preventing indefinite blocking of the main thread.
• Reduced Initial P2P Sync Timeout: The initial P2P synchronization timeout in initFromP2PWithRetry has been reduced from 10 minutes to 5 minutes, allowing background retries to commence sooner if the initial attempt fails.
• Graceful Shutdown of Background Operations: Added a call to bgCancel() in the Stop method of SyncService to ensure that any ongoing background initialization retries are properly terminated when the service is shut down.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Claude finished @julienrbrt's task in 1m 59s —— View job

Code Review Complete

I've completed a comprehensive review of this PR. The changes introduce a background retry mechanism for P2P initialization when the initial synchronous attempt times out. Overall, this is a valuable improvement for resilience, but I've identified several issues that need attention.

Summary

What Changed:

• Added background context (bgCtx, bgCancel) for managing background operations
• Extracted P2P initialization logic into reusable tryInit() method
• Implemented retryInitInBackground() for continuous background retry attempts with exponential backoff
• Reduced initial P2P sync timeout from 10 minutes to 2 minutes
• Added proper cleanup in Stop() method
• Added comprehensive test coverage for background retry mechanism

Impact: This change prevents nodes from getting stuck indefinitely during P2P initialization, improving startup reliability and resilience when peers are temporarily unavailable.

Critical Issues

1. Race Condition in Store Initialization 🔴

pkg/sync/sync_service.go:423-428

The storeInitialized flag is checked with CompareAndSwap in tryInit(), which is called both from the main Start() flow and from the background goroutine. This creates a race condition:

if syncService.storeInitialized.CompareAndSwap(false, true) {
    if _, err := syncService.initStore(ctx, trusted); err != nil {
        syncService.storeInitialized.Store(false)
        return false, fmt.Errorf("failed to initialize the store: %w", err)
    }
}

Problem: If the initial sync times out at 2 minutes and background retry starts, but then a call to WriteToStoreAndBroadcast() happens (e.g., from an aggregator producing its first block), both code paths could race to initialize the store.

Impact: Potential data corruption or panics from concurrent store operations.

Recommendation: Add a mutex to serialize store initialization or use a more robust synchronization pattern like sync.Once. Fix this →

2. Goroutine Leak Risk 🔴

pkg/sync/sync_service.go:465

The background goroutine is launched with go syncService.retryInitInBackground() but there's no guarantee it will exit cleanly in all scenarios:

case <-timeoutTimer.C:
    syncService.logger.Warn().Err(err).Msg("timeout reached while trying to initialize the store, scheduling background retry")
    go syncService.retryInitInBackground()
    return nil

Problem: If retryInitInBackground() encounters a panic or if the context is never cancelled (e.g., if Stop() is never called due to a crash), the goroutine may leak.

Recommendation:

1. Add panic recovery in the background goroutine
2. Add a defer statement to log when the goroutine exits
3. Consider using a WaitGroup to track background goroutines

Fix this →

3. Context Misuse in tryInit 🟡

pkg/sync/sync_service.go:490

The background retry uses syncService.bgCtx when calling tryInit(), but this context is independent of the service's operational context:

ok, err := syncService.tryInit(syncService.bgCtx)

Problem: If the syncer is started with bgCtx, it may continue running even after the main service context is cancelled, leading to orphaned operations.

Recommendation: The syncer should use the service's main operational context, not the background context. Consider passing both contexts or creating a merged context.

Major Issues

4. No Observability for Background Retry State 🟡

pkg/sync/sync_service.go:478-503

There's no way to observe whether the background retry is running, how many retries have occurred, or when it succeeds/fails.

Recommendation:

1. Add metrics: retry attempts, success/failure counts, time-to-success
2. Add an atomic flag to track if background retry is active
3. Expose retry state via a method for monitoring

Fix this →

5. Timeout Value Lacks Configuration 🟡

pkg/sync/sync_service.go:449

The 2-minute timeout is hardcoded:

timeoutTimer := time.NewTimer(time.Minute * 2)

Problem: Different deployments may need different timeout values based on network conditions, DA layer characteristics, etc.

Recommendation: Make this configurable via config.Config with a sensible default (2 minutes).

6. Test Uses Hard Sleep Instead of Proper Synchronization 🟡

pkg/sync/sync_service_test.go:280

// Give the retry a moment to start and fail at least once (no headers available yet)
time.Sleep(100 * time.Millisecond)

Problem: This test is flaky and time-dependent. On slower CI systems, 100ms might not be enough; on faster systems, it wastes time.

Recommendation: Use proper synchronization (channels, mock callbacks, or monitoring atomic flags) instead of sleeps.

Minor Issues

7. Inconsistent Error Logging 🔵

pkg/sync/sync_service.go:458,496

The retry loop logs errors at Info level, but these are actually operational issues that warrant Warn or Error:

syncService.logger.Info().Err(err).Dur("retry_in", backoff).Msg("headers not yet available from peers, waiting to initialize header sync")
syncService.logger.Info().Err(err).Dur("retry_in", backoff).Msg("background retry: headers not yet available from peers")

Recommendation: Use Warn level for persistent initialization failures to make them visible in production logs.

8. Missing Documentation for Background Context Fields 🔵

pkg/sync/sync_service.go:65-67

// context for background operations
bgCtx    context.Context
bgCancel context.CancelFunc

Recommendation: Expand the comment to explain:

• What "background operations" means specifically
• Lifecycle (created in constructor, cancelled in Stop)
• Why it's separate from the service's operational context

9. Magic Numbers in Backoff Configuration 🔵

pkg/sync/sync_service.go:479-480

backoff := 15 * time.Second
maxBackoff := 5 * time.Minute

Recommendation: Extract these as named constants with comments explaining the rationale for these values.

Positive Aspects ✅

1. Good separation of concerns - Extracting tryInit() improves code modularity and testability
2. Proper resource cleanup - bgCancel() is called in Stop() to ensure clean shutdown
3. Reasonable backoff strategy - Exponential backoff with max cap prevents overwhelming the network
4. Good test coverage - TestBackgroundRetryEventuallySucceeds validates the happy path
5. Non-breaking change - Maintains backward compatibility while improving resilience

Code Quality Assessment

Recommendations

Must Fix Before Merge:

1. ✅ Fix the store initialization race condition (ABCI interface #1)
2. ✅ Add panic recovery to prevent goroutine leaks (Peer discovery #2)
3. ✅ Fix context handling in tryInit (Transaction gossiping #3)

Should Fix Before Merge:
4. Add observability/metrics for background retry (#4)
5. Make timeout configurable (#5)
6. Improve test synchronization (#6)

Nice to Have:
7. Improve error logging levels (#7)
8. Enhance documentation (#8)
9. Extract magic numbers to constants (#9)

Overall Assessment

This PR addresses an important operational issue (nodes getting stuck during P2P init), and the approach is sound. However, there are critical concurrency issues that must be addressed before merging. The race condition in store initialization (#1) could lead to data corruption, and the goroutine leak risk (#2) could cause resource exhaustion in long-running deployments.

After addressing the critical issues, this will be a solid improvement to the codebase's resilience.

Codecov Report

❌ Patch coverage is 53.06122% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.75%. Comparing base (f70e6da) to head (1baacf9).

@@            Coverage Diff             @@
##             main    #3002      +/-   ##
==========================================
+ Coverage   58.58%   58.75%   +0.17%     
==========================================
  Files         110      110              
  Lines       10396    10419      +23     
==========================================
+ Hits         6090     6122      +32     
+ Misses       3662     3651      -11     
- Partials      644      646       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.